Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable linker relaxed addressing when loading gp #137

Merged
merged 1 commit into from
Aug 10, 2023

Conversation

jnippula
Copy link

No description provided.

@jlaitine
Copy link

Sorry guys, I updated our nuttx baseline to be on top of released 12.1.0, so now these PR's need rebase...

@jnippula
Copy link
Author

Need to rebase this after sbi code relocation is done, as this code is also relocated into different source file.

@jnippula jnippula marked this pull request as draft June 27, 2023 07:32
Copy link

@eenurkka eenurkka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried this, works after the relocation changes. I wonder what was the original reason to not have this way, maybe at mpfs_head.S (we have it there, is the other unnecessary in this patch?):
.option push
.option norelax
la gp, __global_pointer$
.option pop

got it wrong (in mpfs_head.S), or maybe a different toolchain was used, or something else I don't know.

@pussuw
Copy link

pussuw commented Aug 9, 2023

Every time the global pointer is loaded, linker relaxation needs to be disabled. This can break randomly, if __global_pointer$ is placed within gp-relative load addresses.

The only way to ensure gp is not loaded via gp-relative (relaxed) loads, is to explicitly tell the linker not to use relaxation.

Linux trap entry does exactly this, it (re-)loads gp when entering kernel. Semantically this is the same thing as going between opensbi <-> kernel.

https://github.com/torvalds/linux/blob/13b9372068660fe4f7023f43081067376582ef3c/arch/riscv/kernel/entry.S#L78

Same is done when initializing the kernel:

https://github.com/torvalds/linux/blob/13b9372068660fe4f7023f43081067376582ef3c/arch/riscv/kernel/head.S#L115

@jnippula
Copy link
Author

jnippula commented Aug 9, 2023

Tried this, works after the relocation changes. I wonder what was the original reason to not have this way, maybe at mpfs_head.S (we have it there, is the other unnecessary in this patch?): .option push .option norelax la gp, __global_pointer$ .option pop

got it wrong (in mpfs_head.S), or maybe a different toolchain was used, or something else I don't know.

Yes, looks like the gp is already set before the mpfs_opensbi_prepare_hart() is called, so in that sense the gp loading in _prepare_hart function could be removed.

@pussuw
Copy link

pussuw commented Aug 9, 2023

Tried this, works after the relocation changes. I wonder what was the original reason to not have this way, maybe at mpfs_head.S (we have it there, is the other unnecessary in this patch?): .option push .option norelax la gp, __global_pointer$ .option pop
got it wrong (in mpfs_head.S), or maybe a different toolchain was used, or something else I don't know.

Yes, looks like the gp is already set before the mpfs_opensbi_prepare_hart() is called, so in that sense the gp loading in _prepare_hart function could be removed.

It all depends if opensbi uses gp relative instructions. I'm more concerned about mpfs_exception_opensbi.

I think our bootloader uses gp-relative instructions but it sets gp in mpfs_head.S so in this case setting it (again) in mpfs_opensbi_prepare_hart() should be unnecessary.

Copy link

@pussuw pussuw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jnippula jnippula marked this pull request as ready for review August 10, 2023 06:30
@jnippula jnippula merged commit c252954 into master Aug 10, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants